Could maybe considering a FileProperties class? This would mean you wouldn't need the SendFileAsync overloads/extensions.

Or as simple as SendFileOptions. Make it a struct if you're terribly worried about allocations.

Can we have a way to upload multiple files per message, too? I know this is supported (mobile clients do it!). Perhaps an overload taking an IEnumerable<SendFileOptions>.

I think a properties class is much cleaner.

I also lean towards CreateInviteProperties or CreateInviteOptions.

So, should we do the same for stuff like ChannelPermissions, lest we forget the best url: https://docs.stillu.cc/api/Discord.ChannelPermissions.html#Discord_ChannelPermissions__ctor_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_

IsNews property, yes.

Same comment as above. If there is no functional/operational difference in the API, a property is simpler.

So what happens when discord decides to add (more) special functionality to news channels? (they can switch between news and text types) Or, are we still sticking to treating them just like text channels and not supporting them (like store channels)?

There should at least be an id version of it on IGuild.

That thought should apply to almost all API functionality. Except, perhaps, when there should be a "does this ID exist" check, in which case performing the Get() call serves that purpose.

It is a waste, yes.

I can find no usage of it that I wouldn't be happy to replace.

Would it be better to make this a ushort and people can just ToString() it if needed? I feel like it could stay consistent since discriminators are numbers.

I think that it would be most appropriate to keep Discriminators strings to match the API: https://discordapp.com/developers/docs/resources/user#user-object
There's less risk of things breaking should they change what discriminators can be.

Isn't this meant to be internal?

the EventArgs pattern is probably better.

EventArgs or similar please. For one, we do basically that anyway, in a wrapper layer around the client. For two, it's WAY better from a documentation standpoint. For example, GuildMemberUpdated today has a signature of

public event Func<SocketGuildUser, SocketGuildUser, Task> GuildMemberUpdated;

If I don't already know how this event works, and want to know, which user is the "old" version and which is the "new" one, the only way to really know for sure is to go dig through the source on github and see that when it's actually invoked, it's...

await TimedInvokeAsync(_guildMemberUpdatedEvent, nameof(GuildMemberUpdated), before, user).ConfigureAwait(false);

Yes. As long as the immutable collections aren't used (they're very slow and memory heavy).

Not sure I agree with that assessment of immutables, in general, but I definitely agree that IReadOnlyCollection is the best bet here. Immutables would be appropriate if you KNOW your consumer is going to be doing a lot of collection manipulation, and also wants immutability.

The other option would be IReadOnlyList but do we really expect consumers to want indexing by position?

Does this mean that TokenType is dead, and will be assumed based on the type of client that is used?

Not specifically, I just hadn't implemented it yet. Though, this client will only support bot tokens, as it makes more sense for OAuth users to just use Wumpus directly.

Are the sharded client and websocket client going to be merged?

Since we're just wrapping over a WumpusGatewayClient and not actually implementing any client logic ourselves here, I think it seems reasonable to allow an IDiscordClient to support multiple shards.

So, should we do the same for stuff like ChannelPermissions, lest we forget the best url: https://docs.stillu.cc/api/Discord.ChannelPermissions.html#Discord_ChannelPermissions__ctor_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_System_Boolean_

timespan?

Should this consume an EmoteBuilder instead?

EmoteBuilder is a static class for constructing IEmote/IGuildEmote from strings; see above comment regarding DiscordClientBuilder. I'll probably just replace this with IEmote.Parse/IGuildEmote.Parse

Timespan?

I don't see any point in wrapping imo

nit: thoughts on a DisplayName extension that does Nickname ?? Username, since it's asked for so often

Seconded.

sounds good to me, I guess we can just make this a default interface implementation, since it doesn't look like extension everything is going to make the cut for c#8

I think it's appropriate to split the different types of entities via the typing system, rather than simple properties, when the set of functionality for that entity diverges. I.E. when there are operations that can be performed only upon a category channel, and operations that cannot be performed upon a category channel, even if they don't appear directly upon the interface. Such operations can then be defined to operate specifically upon ICategoryChannel, which provides a more self-documented, foolproof API.

If no such operations exist for ICategoryChannel then it probably makes more sense as just a property.

Or as simple as SendFileOptions. Make it a struct if you're terribly worried about allocations.

I also lean towards CreateInviteProperties or CreateInviteOptions.

Same comment as above. If there is no functional/operational difference in the API, a property is simpler.

EventArgs or similar please. For one, we do basically that anyway, in a wrapper layer around the client. For two, it's WAY better from a documentation standpoint. For example, GuildMemberUpdated today has a signature of

public event Func<SocketGuildUser, SocketGuildUser, Task> GuildMemberUpdated;

If I don't already know how this event works, and want to know, which user is the "old" version and which is the "new" one, the only way to really know for sure is to go dig through the source on github and see that when it's actually invoked, it's...

await TimedInvokeAsync(_guildMemberUpdatedEvent, nameof(GuildMemberUpdated), before, user).ConfigureAwait(false);

Are events no longer asynchronous? If not, presumably we become responsible for dispatching the event to a new async context, if desired?

I'm totally cool with that, since that's what we do currently anyway, but it might be a good idea to have a built-in mechanism to do said dispatching, for beginner consumers. Something like an injectable IDiscordEventDispatcher, with a couple built-in implementations?

Yes, users are now responsible for dispatching to an async context. (see the included example)

I'm not familiar with what you mean by providing something like an IDiscordEventDispatcher, if you could link me to an example of how that might look I would be happy to include it.

In my mind, the ideal way to handle discord events is the same way ASP.NET handles HTTP requests: each incoming message spawns a new "logical" thread. I.E. a new IServiceScope in the DI system, and a spawned Task that is simply not awaited, so that it runs in parallel to other "logical" threads.

For the current Discord.NET version, we do this with several pieces fitting together:
https://github.com/discord-csharp/MODiX/blob/master/Modix.Common/Messaging/MessageDispatcher.cs
https://github.com/discord-csharp/MODiX/blob/master/Modix.Common/Messaging/INotificationHandler.cs
https://github.com/discord-csharp/MODiX/blob/master/Modix.Services/Core/DiscordSocketListeningBehavior.cs

You can see how subscribing to each event we need, wrapping up the args into an object, and then handing off the work to the dispatcher is a tad tedious.

An IDiscordEventDispatcher could look like the following...

public interface IDiscordEventDispatcher
{
    Task DispatchEvent(IEnumerable<Func<Task>> eventHandlers);
}

Where you could have the following built-in implementations that provide behavior similar to Discord.Commands.RunMode.

public class SyncDiscordEventDispatcher
    : IDiscordEventDispatcher
{
    public Task DispatchEvent(IEnumerable<Func<Action>> eventHandlers)
    {
        foreach(var eventHandler in eventHandlers)
            await eventHandler.Invoke();
    }
}

public class ParallelDiscordEventDispatcher
    : IDiscordEventDispatcher
{
    public Task DispatchEvent(IEnumerable<Func<Task>> eventHandlers)
    {
        foreach(var eventHandler in eventHandlers)
            _ = eventAction.Invoke();
        return Task.CompletedTask;
    }
}

You can throw in some basic error handling around the Tasks that aren't awaited, or leave that to the consumer.

In our case, we'd implement a custom dispatcher that also handles DI scoping, since Discord.NET doesn't have any built-in DI. Which, actually, now that I think about it, wouldn't fit with the above interface definition.

So, if you're interested at all, it definitely needs some brainstorming. But, I think the concept of handling messages the way ASP.NET handles HTTP requests would be appealing to a lot of consumers.

This makes me smile.

Not sure I agree with that assessment of immutables, in general, but I definitely agree that IReadOnlyCollection is the best bet here. Immutables would be appropriate if you KNOW your consumer is going to be doing a lot of collection manipulation, and also wants immutability.

The other option would be IReadOnlyList but do we really expect consumers to want indexing by position?

Possible need for clarification: What's the difference between SyncOnly and CacheOnly?

SyncOnly requests (can't enforce) that a state provider never generates an async-state machine, since all state lookups are ValueTasks.

CacheOnly requests that a state provider doesn't contact Discord when retrieving an entity.

The motivation for implementing this might be if you're using an additional caching layer for long-term storage, such as redis, where a state provider might need to make an async call to retrieve information. If you're in a context where the ValueTask must remain synchronous (maybe in a TypeReader, or if you're .Result'ing), you could use SyncOnly.

That's an excellent thought. You should probably just take that last paragraph and inject it into the XML.

What's the logic behind having these defined as events? Personally, I'd much prefer something like exposing the AsyncEvent<T> fields from v2 (which are private, and used internally anyway). This way, you can work with events as first-class objects, and e.g. do something like public async Task<T> WaitFor(this AsyncEvent<T>) like in discord.py - super useful for making interactive stuff.